Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CircleCI Fix: Remove merged strings from pending.ftl #4538

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

rafeerahman
Copy link
Contributor

This fixes failing CircleCI tests by removing pending.ftl strings that were merged in c61dbed, which caused the tests to break.

Copy link
Contributor

@say-yawn say-yawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, the strings that were temporarily in the pending.ftl files have been merged in the l10n repo.

(suggestion, non-blocking) The instructions on README for pending strings only mentions:

If you're not yet ready to submit some strings for translation, you can tentatively add them to frontend/pendingTranslations.ftl. Strings in that file will show up until strings with the same ID are added to the l10n repository.

Should we add another line in README and on pending.ftl indicating pending strings should be removed after string is merged in the l10n repo?

@rafeerahman
Copy link
Contributor Author

Changes look good, the strings that were temporarily in the pending.ftl files have been merged in the l10n repo.

(suggestion, non-blocking) The instructions on README for pending strings only mentions:

If you're not yet ready to submit some strings for translation, you can tentatively add them to frontend/pendingTranslations.ftl. Strings in that file will show up until strings with the same ID are added to the l10n repository.

Should we add another line in README and on pending.ftl indicating pending strings should be removed after string is merged in the l10n repo?

Yes, this will be a good addition. I recall this same issue happened before because pending strings were kept. Do you want me to add that change in this PR?

@say-yawn
Copy link
Contributor

(comment) Interesting to note that the frontend/pendingTranslations.ftl still has old strings that are merged but does not error. I wonder if there is a configuration to use the l10n repo strings if the string is found in both pending and in translations repo.

@say-yawn
Copy link
Contributor

@rafeerahman, adding the comments in this PR would be helpful.

@rafeerahman
Copy link
Contributor Author

(comment) Interesting to note that the frontend/pendingTranslations.ftl still has old strings that are merged but does not error. I wonder if there is a configuration to use the l10n repo strings if the string is found in both pending and in translations repo.

Yeah, the issue seems to be in https://github.com/mozilla/fx-private-relay/blob/main/privaterelay/ftl_bundles.py#L29-L40, where Bundle() causes an error if there are duplicate strings from the files listed. Would be nice if we could have a configuration to ignore/fix this.

@rafeerahman rafeerahman added this pull request to the merge queue Mar 25, 2024
Merged via the queue into main with commit eaf97a4 Mar 25, 2024
28 checks passed
@rafeerahman rafeerahman deleted the fix-circle-2024-03-25 branch March 25, 2024 19:56
@Vinnl
Copy link
Collaborator

Vinnl commented Mar 26, 2024

(comment) Interesting to note that the frontend/pendingTranslations.ftl still has old strings that are merged but does not error. I wonder if there is a configuration to use the l10n repo strings if the string is found in both pending and in translations repo.

Yep, the frontend only uses the pending strings if the IDs aren't found among the l10n repo's strings, but otherwise ignores them - i.e. they're a fallback only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants